fix(index): isolate null-only zone maps in version 1 - #8190
Conversation
|
Important This PR touches the Lance format specification. Substantive changes to the format specification — the If this is a meaningful format change:
|
4878e14 to
4b3e593
Compare
4b3e593 to
0271a1c
Compare
0271a1c to
9e3d816
Compare
westonpace
left a comment
There was a problem hiding this comment.
This is a great follow-up, a nice extensive set of tests (I appreciate that it even has a migration test) and I think a worthy use of a new version.
It is a spec change, so we will need a vote. I'll get that started and try to come back later and look at this with more detail.
| #[derive(Clone, Copy)] | ||
| #[repr(u32)] | ||
| enum ZoneMapIndexVersion { | ||
| Ordered = 0, | ||
| NullOnly = 1, | ||
| } |
There was a problem hiding this comment.
Are you saying that version 1 is only used for the null-only case? In other words, a writer will choose version 0 or version 1 based on the data type?
I think we want version numbers to be more of an increasing, inclusive concept. In other words...
Version 0 does not know how to create null-only zone maps.
Version 1 can create everything version 0 can and can also do null-only zone maps.
Is this a correct understanding?
There was a problem hiding this comment.
Yes, that is the intended compatibility model. ZoneMapIndexPlugin::version() reports the maximum version this implementation supports (1), while each created index records the minimum reader version required by its physical layout. Ordered indices therefore continue to write version 0, and null-only indices write version 1. A version-1 implementation can read and create both layouts; a version-0 reader retains ordered indices and ignores null-only indices so it falls back to scanning.
| fn serialize_data_type(data_type: &DataType) -> Result<bytes::Bytes> { | ||
| let schema = Arc::new(arrow_schema::Schema::new(vec![Field::new( | ||
| "value", | ||
| data_type.clone(), | ||
| true, | ||
| )])); | ||
| let mut buffer = Cursor::new(Vec::new()); | ||
| let mut writer = FileWriter::try_new(&mut buffer, &schema)?; | ||
| writer.finish()?; | ||
| Ok(bytes::Bytes::from(buffer.into_inner())) | ||
| } |
There was a problem hiding this comment.
Why do we need to store the data type in the index?
There was a problem hiding this comment.
Null-only zone maps store min and max physically as Arrow Null, so their file schema no longer carries the indexed logical type. The scalar-index loader receives the index store and details, but not the dataset field, and the logical type is needed after loading to validate updates and rebuild the correct processor/seeds. The data_type global buffer preserves it as a one-field Arrow IPC schema. Ordered version-0 maps still infer the type from min/max and do not write this buffer.
| Field::new("null_count", DataType::UInt32, false), | ||
| Field::new("nan_count", DataType::UInt32, false), | ||
| Field::new("zone_length", DataType::UInt64, false), | ||
| Field::new("null_offsets", DataType::Binary, false), |
There was a problem hiding this comment.
null_offsets contains the exact top-level-null row positions relative to the start of each seed zone, encoded as little-endian u64 values in the binary field. null_count alone is insufficient to reconstruct the complete RowAddrTreeMap when an append/update is built from seeds. During seed loading, each zone-relative offset is combined with the zone start and fragment ID to recover the absolute row address. Legacy seeds without this field deliberately fall back to the scanned update path.
9e3d816 to
cdc8283
Compare
8700a32 to
edbee84
Compare
Keep dense-null seed payloads compact while validating decoded offsets and documenting zone span and null-query guarantees.
edbee84 to
20b88ff
Compare
Trim unsupported extrema handling, speculative seed validation, and redundant test matrices while preserving compatibility and correctness coverage.
Keep ordered zone maps on version 0 while giving nested null-only layouts an explicit format boundary that older readers safely reject.
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
The ordered-v0/null-only-v1 boundary is sound, and the earlier seed concern is no longer attributable to this diff. One current-diff loader regression remains: malformed stable-format data can panic before fallible schema validation runs.
Keep v0 type discovery and required v1 metadata validation on an error-returning path. After that, maintainers still need to complete the Zone Map Version 1 vote.
| ZoneMapMode::Ordered | ||
| } | ||
| }); | ||
| let data_type = persisted_data_type.unwrap_or_else(|| zone_maps["min"].data_type().clone()); |
There was a problem hiding this comment.
This fallback indexes the batch by name before try_from_serialized_with_mode reaches its fallible schema checks. RecordBatch string indexing unwraps a missing column, so an otherwise parseable v0 index without min now panics the process; the same file returned Err at the base revision. Use a fallible column_by_name("min") lookup for ordered fallback, and require/validate data_type for null-only mode instead of silently adopting physical Null.
Reproducer
#[tokio::test]
async fn test_missing_min_returns_error() {
let tmpdir = TempObjDir::default();
let store = Arc::new(LanceIndexStore::new(
Arc::new(ObjectStore::local()),
tmpdir.clone(),
Arc::new(LanceCache::no_cache()),
));
let schema = Arc::new(Schema::new(vec![
Field::new("max", DataType::Int32, true),
Field::new("null_count", DataType::UInt32, false),
Field::new("nan_count", DataType::UInt32, false),
Field::new("fragment_id", DataType::UInt64, false),
Field::new("zone_start", DataType::UInt64, false),
Field::new("zone_length", DataType::UInt64, false),
]));
let batch = RecordBatch::try_new(schema.clone(), vec![
Arc::new(Int32Array::from(vec![Some(99)])) as _,
Arc::new(UInt32Array::from(vec![0])) as _,
Arc::new(UInt32Array::from(vec![0])) as _,
Arc::new(UInt64Array::from(vec![0])) as _,
Arc::new(UInt64Array::from(vec![0])) as _,
Arc::new(UInt64Array::from(vec![1])) as _,
]).unwrap();
let mut writer = store.new_index_file(ZONEMAP_FILENAME, schema).await.unwrap();
writer.write_record_batch(batch).await.unwrap();
writer.finish().await.unwrap();
let result = ZoneMapIndex::load(store, None, &LanceCache::no_cache(), false).await;
assert!(result.is_err(), "a malformed index must return an error");
}cargo test -p lance-index scalar::zonemap::tests::test_missing_min_returns_error -- --exact --nocapture failed on this head with exit 101: called Option::unwrap() on a None value at this line. The identical test passed on the base revision.
Summary
Main currently writes nested, null-only zone maps as version 0—the same version used by ordered zone maps. A version-0 reader can therefore accept an index whose
minandmaxno longer have ordered semantics, leading to load failures or unsafe pruning.This PR introduces a version-1 null-only layout while leaving the ordered version-0 layout unchanged.
Problem on main
A null-only zone map can answer null predicates, but it has no value range. It must never be interpreted as an ordered zone map.
Solution walkthrough
Orderedand nested zone maps asNullOnly.minandmaxcolumns.Nullextrema columns.data_typeglobal buffer because ArrowNullcolumns do not carry it.supports_min_maxinZoneMapIndexDetails; an absent field defaults to ordered for existing version-0 indices.IN, and prefix predicates fall back to ordinary scanning.Scope
This PR is intentionally limited to the null-only version-1 format boundary. Ordered extrema enhancements—including Decimal, FixedSizeBinary, Dictionary, and NaN handling—are deferred to follow-up PRs.
Test plan
cargo fmt --allcargo check -p lance-index --testscargo clippy -p lance-index --all-targets -- -D warningscargo test -p lance-index scalar::zonemap::tests --no-fail-fast(36 passed)cargo test -p lance-index test_null_only --no-fail-fast(2 passed)Format vote: #8302